Code Review Summary

Status: 2 Observations | Recommendation: Review task placement before merge

Overview

Issue Alignment Check: This PR implements the changes described in #608 - adding celery beat schedules for expiration reminders and user deactivation tasks.

Testing Recommendation: Since this PR modifies celery beat schedules and moves tasks between modules, ensure integration tests verify:

1. The tasks execute correctly in both RADIUS and non-RADIUS installations
2. No duplicate task registrations occur
3. The reminder email task exists in the openwisp-users package

_{Reviewed by gpt-5.4-20260305 · 19,685 tokens}

No actionable comments were generated in the recent review. 🎉

sequenceDiagram
  participant SettingsTemplate
  participant CeleryBeat
  participant openwisp_users.deactivate as openwisp_users.tasks.deactivate_expired_users
  participant openwisp_users.reminder as openwisp_users.tasks.expiration_reminder_email
  participant openwisp2_radius

  SettingsTemplate->>CeleryBeat: register deactivate_expired_users (crontab)
  SettingsTemplate->>CeleryBeat: register expiration_reminder_email (crontab)
  CeleryBeat->>openwisp_users.deactivate: run at 00:01
  CeleryBeat->>openwisp_users.reminder: run at 00:03
  SettingsTemplate->>openwisp2_radius: remove radius-scoped deactivate task

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3).

The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3).

The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (3/3).

Molecule Destroy Failure

Hello @pandafy,
(Analysis for commit f3e0020)

The Molecule destroy scenario failed because the instances could not be deleted. This is likely a transient infrastructure issue. The CI has been restarted automatically. If the issue persists, please report it.

The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3).

The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3).

The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3).

The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3).

@coderabbitai full review!

#!/bin/bash
# Read all changed files
echo "=== defaults/main.yml ==="
cat defaults/main.yml

echo ""
echo "=== templates/openwisp2/settings.py ==="
cat templates/openwisp2/settings.py

echo ""
echo "=== docs/user/role-variables.rst ==="
cat docs/user/role-variables.rst

echo ""
echo "=== PR diff ==="
gh pr diff 615 --repo openwisp/ansible-openwisp2

@pandafy: Here is a full review of the changes in this PR.

Overall Assessment

The PR correctly moves deactivate_expired_users from openwisp_radius.tasks to openwisp_users.tasks (making it unconditional and available even without RADIUS), adds the new expiration_reminder_email periodic task, and importantly adds CELERY_TIMEZONE = TIME_ZONE to keep Celery crontab schedules aligned with Django's timezone. The changes are clean and the documentation is kept in sync.

A few issues worth addressing:

Issue 1 — "args": None should be omitted

File: templates/openwisp2/settings.py

"deactivate_expired_users": {
    "task": "openwisp_users.tasks.deactivate_expired_users",
    "schedule": crontab(**{ {{ cron_deactivate_expired_users }} }),
    "args": None,   # ← non-standard
},

In Celery beat, the args key is expected to be a tuple or list. Passing None can cause a TypeError depending on the Celery version (TypeError: 'NoneType' object is not iterable). The original radius-scoped entry also had "args": None, so this bug was carried over. The fix is simply to omit the key:

"deactivate_expired_users": {
    "task": "openwisp_users.tasks.deactivate_expired_users",
    "schedule": crontab(**{ {{ cron_deactivate_expired_users }} }),
},

Issue 2 — Missing openwisp2_users_periodic_tasks toggle (consistency)

Both openwisp2_monitoring_periodic_tasks and openwisp2_radius_periodic_tasks exist to allow operators to disable periodic tasks for those modules. The two new openwisp_users tasks are always registered unconditionally, breaking this pattern:

{% if openwisp2_radius and openwisp2_radius_periodic_tasks %}
    "delete_old_radiusbatch_users": { ... },
{% endif %}

Consider adding a openwisp2_users_periodic_tasks: true variable in defaults/main.yml and guarding the two new tasks behind it:

{% if openwisp2_users_periodic_tasks %}
    "deactivate_expired_users": { ... },
    "expiration_reminder_email": { ... },
{% endif %}

This gives operators the same flexibility they have for the other modules.

Issue 3 — Variable ordering in defaults/main.yml is slightly misleading

The variables are listed in this order:

cron_delete_old_notifications: "'hour': 0, 'minute': 0"
cron_expiration_reminder_email: "'hour': 0, 'minute': 3"   # runs at :03
cron_deactivate_expired_users: "'hour': 0, 'minute': 1"    # runs at :01

cron_deactivate_expired_users (minute 1) runs before cron_expiration_reminder_email (minute 3), but it appears second in the file. This is a minor readability nit — listing them in chronological order (minute 1 then minute 3) would make the scheduling intent clearer.

Minor — CELERY_TIMEZONE placement

CELERY_TIMEZONE = TIME_ZONE is placed after TIME_ZONE which is correct. One small thing: the blank line added after it creates a double blank line before {% if openwisp2_internationalization %}. Not a functional issue, just cosmetic.

Note on upstream blockers

The PR mentions it depends on openwisp-users/pull/517 and openwisp-radius/pull/719. Make sure the openwisp_users.tasks.expiration_reminder_email and updated openwisp_users.tasks.deactivate_expired_users task signatures are finalised in those PRs before merging this one, to avoid a broken deployment for anyone pinned to a recent release.

Actionable comments posted: 0